Skip to content

Conversation

@psainics
Copy link
Contributor

@psainics psainics commented Jan 13, 2025

Error Management catch known errors

Jira : PLUGIN-1842

Description

  • Remove unused imports
  • Error Management catch known errors

Code change

  • Modified GCSBucketCreate.java

Tests

  • Test Case (Try to create a bucket with no write access on it)
Raw Logs
2025-01-13 15:38:06,370 - ERROR [WorkflowDriver:i.c.c.i.a.r.w.WorkflowProgramController@90] - Workflow service 'workflow.default.GCS_BCREATE_TEST_RUN.DataPipelineWorkflow.3e5879da-d196-11ef-a658-000000579539' failed.
io.cdap.cdap.api.exception.WrappedStageException: Stage 'GCS Create' encountered : io.cdap.cdap.api.exception.ProgramFailureException: Unable to access or create bucket this_is_not_my_bucket. Ensure you entered the correct bucket path and have permissions for it. [email protected] does not have storage.buckets.get access to the Google Cloud Storage bucket. Permission 'storage.buckets.get' denied on resource (or it may not exist).
  at io.cdap.cdap.etl.common.plugin.ExceptionWrappingCaller.call(ExceptionWrappingCaller.java:64)
  at io.cdap.cdap.etl.common.plugin.WrappedAction.run(WrappedAction.java:48)
  at io.cdap.cdap.etl.batch.customaction.PipelineAction.run(PipelineAction.java:91)
  at io.cdap.cdap.internal.app.runtime.AbstractContext.execute(AbstractContext.java:608)
  at io.cdap.cdap.internal.app.runtime.workflow.CustomActionExecutor.execute(CustomActionExecutor.java:90)
  at io.cdap.cdap.internal.app.runtime.workflow.WorkflowDriver.executeCustomAction(WorkflowDriver.java:449)
  at io.cdap.cdap.internal.app.runtime.workflow.WorkflowDriver.executeNode(WorkflowDriver.java:489)
  at io.cdap.cdap.internal.app.runtime.workflow.WorkflowDriver.executeAll(WorkflowDriver.java:669)
  at io.cdap.cdap.internal.app.runtime.workflow.WorkflowDriver.run(WorkflowDriver.java:653)
  at com.google.common.util.concurrent.AbstractExecutionThreadService$1$1.run(AbstractExecutionThreadService.java:52)
  at java.lang.Thread.run(Thread.java:750)
Caused by: io.cdap.cdap.api.exception.ProgramFailureException: Unable to access or create bucket this_is_not_my_bucket. Ensure you entered the correct bucket path and have permissions for it. [email protected] does not have storage.buckets.get access to the Google Cloud Storage bucket. Permission 'storage.buckets.get' denied on resource (or it may not exist).
  at io.cdap.cdap.api.exception.ProgramFailureException$Builder.build(ProgramFailureException.java:236)
  at io.cdap.cdap.api.exception.ErrorUtils.getProgramFailureException(ErrorUtils.java:161)
  at io.cdap.plugin.gcp.gcs.actions.GCSBucketCreate.run(GCSBucketCreate.java:143)
  at io.cdap.cdap.etl.common.plugin.WrappedAction.lambda$run$1(WrappedAction.java:49)
  at io.cdap.cdap.etl.common.plugin.Caller$1.call(Caller.java:30)
  at io.cdap.cdap.etl.common.plugin.ExceptionWrappingCaller.call(ExceptionWrappingCaller.java:62)
  ... 10 common frames omitted
Caused by: com.google.cloud.storage.StorageException: [email protected] does not have storage.buckets.get access to the Google Cloud Storage bucket. Permission 'storage.buckets.get' denied on resource (or it may not exist).
  at com.google.cloud.storage.spi.v1.HttpStorageRpc.translate(HttpStorageRpc.java:233)
  at com.google.cloud.storage.spi.v1.HttpStorageRpc.get(HttpStorageRpc.java:425)
  at com.google.cloud.storage.StorageImpl.lambda$get$4(StorageImpl.java:264)
  at com.google.api.gax.retrying.DirectRetryingExecutor.submit(DirectRetryingExecutor.java:105)
  at com.google.cloud.RetryHelper.run(RetryHelper.java:76)
  at com.google.cloud.RetryHelper.runWithRetries(RetryHelper.java:50)
  at com.google.cloud.storage.Retrying.run(Retrying.java:51)
  at com.google.cloud.storage.StorageImpl.run(StorageImpl.java:1374)
  at com.google.cloud.storage.StorageImpl.get(StorageImpl.java:263)
  at io.cdap.plugin.gcp.gcs.actions.GCSBucketCreate.run(GCSBucketCreate.java:137)
  ... 13 common frames omitted
Caused by: com.google.api.client.googleapis.json.GoogleJsonResponseException: 403 Forbidden
GET https://storage.googleapis.com/storage/v1/b/this_is_not_my_bucket?projection=full
{
"code" : 403,
"errors" : [ {
  "domain" : "global",
  "message" : "[email protected] does not have storage.buckets.get access to the Google Cloud Storage bucket. Permission 'storage.buckets.get' denied on resource (or it may not exist).",
  "reason" : "forbidden"
} ],
"message" : "[email protected] does not have storage.buckets.get access to the Google Cloud Storage bucket. Permission 'storage.buckets.get' denied on resource (or it may not exist)."
}
  at com.google.api.client.googleapis.json.GoogleJsonResponseException.from(GoogleJsonResponseException.java:146)
  at com.google.api.client.googleapis.services.json.AbstractGoogleJsonClientRequest.newExceptionOnError(AbstractGoogleJsonClientRequest.java:118)
  at com.google.api.client.googleapis.services.json.AbstractGoogleJsonClientRequest.newExceptionOnError(AbstractGoogleJsonClientRequest.java:37)
  at com.google.api.client.googleapis.services.AbstractGoogleClientRequest$1.interceptResponse(AbstractGoogleClientRequest.java:428)
  at com.google.api.client.http.HttpRequest.execute(HttpRequest.java:1111)
  at com.google.api.client.googleapis.services.AbstractGoogleClientRequest.executeUnparsed(AbstractGoogleClientRequest.java:514)
  at com.google.api.client.googleapis.services.AbstractGoogleClientRequest.executeUnparsed(AbstractGoogleClientRequest.java:455)
  at com.google.api.client.googleapis.services.AbstractGoogleClientRequest.execute(AbstractGoogleClientRequest.java:565)
  at com.google.cloud.storage.spi.v1.HttpStorageRpc.get(HttpStorageRpc.java:422)
  ... 21 common frames omitted

POST v3/namespaces/{namespace-id}/apps/{app-id}/workflows/DataPipelineWorkflow/runs/{run-id}/classify
[
{
  "stageName": "GCS Create",
  "errorCategory": "Plugin-'GCS Create'",
  "errorReason": "Unable to access or create bucket this_is_not_my_bucket. Ensure you entered the correct bucket path and have permissions for it.",
  "errorMessage": "Unable to access or create bucket this_is_not_my_bucket. Ensure you entered the correct bucket path and have permissions for it. [email protected] does not have storage.buckets.get access to the Google Cloud Storage bucket. Permission 'storage.buckets.get' denied on resource (or it may not exist).",
  "errorType": "USER",
  "dependency": "true"
}
]

image
image

@psainics psainics added the build Trigger unit test build label Jan 13, 2025
@psainics psainics self-assigned this Jan 13, 2025
@psainics psainics marked this pull request as ready for review January 13, 2025 10:56
Comment on lines 92 to 94
context.getFailureCollector().addFailure(String.format("%s %s", errorReason, e.getMessage()), null)
.withStacktrace(e.getStackTrace());
context.getFailureCollector().getOrThrowException();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

collector is already defined at line 73

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated !

credentials = serviceAccount == null ? null : GCPUtils.loadServiceAccountCredentials(serviceAccount,
isServiceAccountFilePath);
} catch (IOException e) {
String errorReason = "Failed to load service account credentials. ";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove whitespace before period.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed !

isServiceAccountFilePath);
} catch (IOException e) {
String errorReason = "Failed to load service account credentials. ";
context.getFailureCollector().addFailure(String.format("%s %s", errorReason, e.getMessage()), null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String.format("%s %s: %s", errorReason, e.getClass().getName(), e.getMessage())

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated !

Comment on lines 134 to 138
String errorReason = String.format("Unable to access or create bucket %s. ", gcsPath.getBucket()) +
"Ensure you entered the correct bucket path and have permissions for it.";
String errorMessage = String.format("%s %s", errorReason, e.getMessage());
throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN),
errorReason, errorMessage, ErrorType.USER, true, e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StorageException returns http error codes. Handle similarly as we do for GoogleJsonResponseException

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we will have to handle it at multiple places i have created a util class GCPErrorDetailsProviderUtil with a function getHttpResponseExceptionDetailsFromChain that checks for HttpResponseException in the cause chain and handles it same as we do it in GCPErrorDetailsProvider.

image

throw new Exception(String.format("Path %s already exists", gcsPath));
String errorReason = String.format("Path %s already exists", gcsPath);
throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN),
errorReason, errorReason, ErrorType.SYSTEM, true, null);
Copy link
Contributor

@itsankit-google itsankit-google Jan 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not a system error, it is a user error as bucket name is provided by user.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to user error.

String errorReason = String.format("Path %s already exists", gcsPath);
throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN),
errorReason, errorReason, ErrorType.SYSTEM, true, null);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove empty line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed !

String errorReason = "Unable to get GCS filesystem handler.";
String errorMessage = String.format("%s %s", errorReason, e.getMessage());
throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN),
errorReason, errorMessage, ErrorType.SYSTEM, false, e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not a system error, they should be unknown errors.

Also does IOException underneath has GoogleJsonResponse, then we should do as we perform in GCPErrorDetailsProvider.

similar comment for all errors in the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to unknown !

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the IOException to getHttpResponseExceptionDetailsFromChain

}
return ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN), errorReason,
String.format("%s %s: %s", errorReason, e.getClass().getName(), errorMessage), pair.getErrorType(), true,
ErrorCodeType.HTTP, statusCode.toString(), GCPUtils.GCS_SUPPORTED_DOC_URL, e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why GCS_SUPPORTED_DOC_URL always, this should be passed as parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

passing this in function param now

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this comment is still not resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved now !

rollback = true;
throw new Exception("Unable to get GCS filesystem handler. " + e.getMessage(), e);
String errorReason = "Unable to get GCS filesystem handler.";
throw GCPErrorDetailsProviderUtil.getHttpResponseExceptionDetailsFromChain(e, errorReason, ErrorType.SYSTEM,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why SYSTEM, these error type should be UNKNOWN by default.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment applies to whole PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated to unknown errors

Comment on lines 168 to 170
String errorReason = String.format("Failed to create path %s.", gcsPath);
throw GCPErrorDetailsProviderUtil.getHttpResponseExceptionDetailsFromChain(e, errorReason, ErrorType.SYSTEM,
true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please add evidence of one such instance where it extracts details from IOException and return correct result in Error Management UI?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't seem to trigger this one.
i theory it should work if there is a HttpResponseException in the cause chain, else the generic exception should be returned.

Test cases i have tried.

  • Debug Pause to create the dir but it does not fail if dir alredy exist
  • Hardcode in incorrect path, fails during validation / IllegalArgumentException

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can add permission to create GCSBucket but do not add permission to create objects it should fail at that step right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally got this working,
image
image
image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does it have whole errorReason copied in errorMessage in the screenshot?

Copy link
Contributor

@itsankit-google itsankit-google left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also do the same for whenever we are creating storage client like on line 115.

@psainics
Copy link
Contributor Author

psainics commented Jan 16, 2025

Please also do the same for whenever we are creating storage client like on line 115.

This is being addressed in a separate PR.

@psainics
Copy link
Contributor Author

Rebased and Squashed

private static ProgramFailureException getProgramFailureException(HttpResponseException e, String externalDocUrl) {
String errorReason = getErrorReason(e, externalDocUrl);
return ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN), errorReason,
String.format("%s %s: %s", errorReason, e.getClass().getName(), getErrorMessage(e)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not copy errorReason in errorMessage, was it done earlier in GCPErrorDetailsProvider?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes i think it;s done in GCPErrorDetailsProvider.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, it was not like that. Please see old code:

String errorReason = String.format("%s %s. %s", e.getStatusCode(), e.getStatusMessage(),
pair.getCorrectiveAction());
String errorMessage = e.getMessage();
String externalDocumentationLink = null;
if (e instanceof GoogleJsonResponseException) {
errorMessage = getErrorMessage((GoogleJsonResponseException) e);
externalDocumentationLink = getExternalDocumentationLink();
if (!Strings.isNullOrEmpty(externalDocumentationLink)) {
if (!errorReason.endsWith(".")) {
errorReason = errorReason + ".";
}
errorReason = String.format("%s For more details, see %s", errorReason,
externalDocumentationLink);
}
}
return ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategoryEnum.PLUGIN),
errorReason, String.format(ERROR_MESSAGE_FORMAT, errorContext.getPhase(),
e.getClass().getName(), errorMessage),
pair.getErrorType(), true, ErrorCodeType.HTTP, statusCode.toString(),
externalDocumentationLink, e);
}

Copy link
Contributor

@itsankit-google itsankit-google left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one comment. PTAL, thanks!

Copy link
Contributor

@itsankit-google itsankit-google left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think current implementation of separating out errorReason and errorMessage is not needed, can you just pull this method in GCPErrorDetailsProviderUtil and call it in both places?

private ProgramFailureException getProgramFailureException(HttpResponseException e,
      @Nullable ErrorContext errorContext) {
    Integer statusCode = e.getStatusCode();
    ErrorUtils.ActionErrorPair pair = ErrorUtils.getActionErrorByStatusCode(statusCode);
    String errorReason = String.format("%s %s. %s", e.getStatusCode(), e.getStatusMessage(),
      pair.getCorrectiveAction());

    String errorMessage = e.getMessage();
    String externalDocumentationLink = null;
    if (e instanceof GoogleJsonResponseException) {
      errorMessage = getErrorMessage((GoogleJsonResponseException) e);

      externalDocumentationLink = getExternalDocumentationLink();
      if (!Strings.isNullOrEmpty(externalDocumentationLink)) {

        if (!errorReason.endsWith(".")) {
          errorReason = errorReason + ".";
        }
        errorReason = String.format("%s For more details, see %s", errorReason,
          externalDocumentationLink);
      }
    }

    return ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategoryEnum.PLUGIN),
      errorReason, errorMessage != null ? String.format(ERROR_MESSAGE_FORMAT, errorContext.getPhase(),
            e.getClass().getName(), errorMessage) : String.format("%s: %s", e.getClass().getName(), errorMessage),
      pair.getErrorType(), true, ErrorCodeType.HTTP, statusCode.toString(),
        externalDocumentationLink, e);
  }

if errorContext is null you can just keep the errorMessage without adding errorPhase information.

@psainics psainics force-pushed the fem/action/GCSBucketCreate branch from 5141087 to dbba9cd Compare January 16, 2025 12:24
@psainics
Copy link
Contributor Author

image
Updated !

}
return ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN), errorReason,
errorContext != null ?
String.format("Error occurred in the phase: '%s'. %s: %s", errorContext.getPhase(), e.getClass().getName(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can make ERROR_MESSAGE_FORMAT package private in GCPErrorDetailsProvider and use here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated !


public static ProgramFailureException getHttpResponseExceptionDetailsFromChain(Exception e, String errorReason,
String externalDocUrl) {
return getHttpResponseExceptionDetailsFromChain(e, errorReason, ErrorType.USER, true, externalDocUrl);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you want to delegate the call to another method here?

Also errorType should be UNKNOWN by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the errors are user error, i did this to not have to write this again , and directly use this function!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For errors that will be unknown we will specify the error type, this is only to be used with error that are caused due to user.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is no ProgramFailureException & HttpResponseException in the causal chain, how do we know it is USER?

@psainics psainics force-pushed the fem/action/GCSBucketCreate branch from dbba9cd to 73a0446 Compare January 16, 2025 12:41
@psainics psainics force-pushed the fem/action/GCSBucketCreate branch 3 times, most recently from cf1e771 to c22e84d Compare January 16, 2025 13:18
@psainics psainics force-pushed the fem/action/GCSBucketCreate branch from c22e84d to bdd55f3 Compare January 16, 2025 14:29
@psainics psainics merged commit a9967d5 into data-integrations:develop Jan 17, 2025
16 checks passed
@psainics psainics deleted the fem/action/GCSBucketCreate branch January 17, 2025 01:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build Trigger unit test build

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants